Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pydantic v2 support #245

Merged
merged 10 commits into from
Sep 4, 2023
Merged

pydantic v2 support #245

merged 10 commits into from
Sep 4, 2023

Conversation

sinisaos
Copy link
Member

Related to piccolo-orm/piccolo_admin#313.

The tests will fail until we pin it to the first version of Piccolo with Pydantic V2 support, when it is released.

@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2023

Codecov Report

Merging #245 (8de3dce) into master (5a7ce77) will increase coverage by 0.21%.
The diff coverage is 92.85%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
+ Coverage   92.50%   92.72%   +0.21%     
==========================================
  Files          33       33              
  Lines        2027     2033       +6     
==========================================
+ Hits         1875     1885      +10     
+ Misses        152      148       -4     
Files Changed Coverage Δ
piccolo_api/shared/auth/junction.py 42.85% <ø> (ø)
piccolo_api/shared/middleware/junction.py 0.00% <0.00%> (ø)
piccolo_api/token_auth/endpoints.py 88.57% <ø> (ø)
piccolo_api/fastapi/endpoints.py 97.74% <90.90%> (-1.45%) ⬇️
piccolo_api/crud/endpoints.py 95.75% <100.00%> (+0.79%) ⬆️
piccolo_api/crud/serializers.py 100.00% <100.00%> (ø)
piccolo_api/crud/validators.py 90.38% <100.00%> (ø)
piccolo_api/media/local.py 100.00% <100.00%> (ø)
piccolo_api/media/s3.py 99.10% <100.00%> (ø)

Comment on lines 416 to 422
for field_name, _field in model.model_fields.items():
# get type of field since field.outer_type_ is
# deprecated in Pydantic V2 (we use that for arrays
# in the OpenAPI docs). Using vars(), but we can
# also use _field.annotation.__dict__ (if that's better)
field_annotation = vars(_field.annotation)
type_ = field_annotation["__args__"][0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the best approach is here. I know we used outer_type because if it's list[int] it returns list.

What does _field.annotation give us?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For array field of strings _field.annotation is typing.Optional[typing.List[str]] but result is wrong

without_args

For array field of strings field_annotation["__args__"][0] is typing.List[str] which is correct

with_args

I don't know how to solve it otherwise, if you have any other idea feel free to write here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this was wrecking my brain.

I think I have a solution now - in Python 3.8 they added typing.get_origin and typing.get_args, which means if we have t.Optional[someType] we're able to extract someType.

Your solution probably would have been fine. I was just worried because for field_annotation["__args__"][0] to work we needed it to always be like (someType, None) and not (None, someType).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried this and the _get_type function doesn't work as it should. The problem remains the same because the Array column does not display correct in the OpenAPI document filters. Now it's like this.

without_args

If I change the _get_type function to something like this

def _get_type(type_: t.Type) -> t.Type:
    """
    Extract the inner type from an optional if necessary, otherwise return
    the type as is.
    """
    args = t.get_args(type_)
    return args[0]

the result is correct and looks like this.

with_args

Can you check that in case I'm wrong or doing something wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right - I can see the problem when trying to use the Director.years_nominated filter in the Swagger docs.

Will investigate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit odd, but t.get_args returns this: (typing.List[int], <class 'NoneType'>) and I was expecting (typing.List[int], None).

I need to work out what NoneType is vs None.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So type(None) is NoneType. I've changed it to check for None and NoneType, and it should work.

Will add some tests.

@sinisaos
Copy link
Member Author

@dantownsend Any updates or new thoughts on this? I think the Piccolo API and Piccolo Admin are equally important parts, as the ORM, in the Piccolo ecosystem and it would be nice if they also have support for Pydantic V2 as soon as possible. Libraries without this support fall out of use, which would be a real shame, because Piccolo is one of the best-maintained, feature-rich libraries out there.

@dantownsend
Copy link
Member

@sinisaos Yeah, I'm running a bit behind of these PRs - has been a busy couple of weeks. But I plan on getting them out asap, because as you say we need them all released, and not just Piccolo.

Copy link
Member

@dantownsend dantownsend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 👍

Sorry it took me so long - I needed a day off to devote to it, so I could wrap my head around some of the Pydantic changes.

@dantownsend dantownsend changed the base branch from master to v1 September 4, 2023 12:42
@dantownsend dantownsend merged commit fde7a4f into piccolo-orm:v1 Sep 4, 2023
11 checks passed
@sinisaos sinisaos deleted the pydantic_v2 branch September 4, 2023 18:39
dantownsend added a commit that referenced this pull request Oct 20, 2023
* pydantic v2 support (#245)

* pydantic_v2_support

* pin to Piccolo v1

* add comment

* remove KeyError Exception

* upgrade coverage

* fix type warnings with `nested`

* fix mypy warnings

* update black

* replacement for `outer_type`

* change assertion

---------

Co-authored-by: Daniel Townsend <[email protected]>

* update github actions

* bumped version

* fix `_get_type` - check for `NoneType` (#254)

* fix `_get_type` - check for `NoneType`

* add tests

* fix typo

* add a test for the new union syntax

* also check for `UnionType`

* bumped version

* update JSON schema (#257)

* bumped version

* Update requirements.txt

---------

Co-authored-by: sinisaos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants